-
Notifications
You must be signed in to change notification settings - Fork 9.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
aws_workspaces_directory: Assign IP Group #14451
aws_workspaces_directory: Assign IP Group #14451
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Tensho, see comments below
@DrFaust92 Thank you for review, I'll make appropriate changes shortly 🙇 |
|
||
func testAccWorkspacesDirectoryConfig_ipGroupIds_update(rName string) string { | ||
return testAccAwsWorkspacesDirectoryConfig_Prerequisites(rName) + fmt.Sprintf(` | ||
resource "aws_workspaces_ip_group" "test_alpha" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DrFaust92 I caught one issue with workspace directory and IP group mutual dependency. If I remove IP group resource and it's reference in workspaces directory declaration, then I get an error like this:
Error: Error Deleting Workspaces IP Group: ResourceAssociatedException: Can not delete IP Group with GroupId - wsipg-5g88k3vby because its attached with these Directories d-9267047639
If I understand correctly that happens, because workspace directory depends on IP group implicitly referencing it in ip_group_ids
, so Terraform deletes dependent resource (IP group) first. But IP group can't be deleted until it is associated with any directory. Sounds like I should declare workspace directory dependency for IP group, but that makes a cycle and violates acyclic graph constraint:
resource "aws_workspaces_ip_group" "test" {
depends_on = ["${aws_workspaces_directory.test.id}"]
}
resource "aws_workspaces_directory" "test" {
directory_id = "${aws_directory_service_directory.test.id}"
ip_group_ids = ["${aws_workspaces_ip_group.test.id}"]
}
I have only one idea – disassociate IP group from directory, but it requires to call DescribeWorkspaceDirectories API action and find the directory associated with IP group at client side – DescribeIpGroups API action doesn't return back reference to directory. Feels not idiomatic. Could you provide an example for already implemented resources with similar behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is something similar in API Gateway Usage plan resource, care to take a look for inspiration?
im guessing there isn't much to do to beside listing all directories and dissociating all directories from that ip group.
maybe it's worth splitting the association itself to a separate resource. and leave the ip_groups in this resource computed.
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, I haven't found anything could help in aws_api_gateway_usage_plan
resource. UpdateUsagePlan
action handles all change together with PatchOperations
attribute, which is designed to handle update operations in ordered manner.
I doubt aws_workspaces_ip_group_attachment
makes any difference and solves the cycle issue.
resource "aws_workspaces_directory" "test" {
directory_id = "${aws_directory_service_directory.test.id}"
}
resource "aws_workspaces_ip_group" "test" {}
resource "aws_workspaces_ip_group_attachment" "test" {
directory_id = "${aws_directory_service_directory.test.id}"
ip_group_ids = ["${aws_workspaces_ip_group.test.id}"]
}
As far as attachment refers to directory and ip group resources, they are destroyed first by Terraform. Nor directory, neither ip group can be destroyed until disassociation is completed.
Seems like we should go with dumb aws_workspaces_ip_group
resource disassociation on Delete()
.
@DrFaust92 Thank you for the insight and please don't hesitate to drop any comment regarding my plan 🙇 I'm not a seasoned Terraform provider developer yet, so appreciate any feedback 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look a the delete function in usage plan. It deletes the associated stages.
I think your plan seems fine to me, @gdavison want if you want to chime in on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just bumped with another idea – reverse (inject) dependency between resources – drop aws_workspaces_directory.ip_group_ids
attribute in favor of aws_workspaces_ip_group.directory_id
one.
resource "aws_workspaces_directory" "test" {
directory_id = "${aws_directory_service_directory.test.id}"
}
resource "aws_workspaces_ip_group" "a" {
directory_id = "${aws_workspaces_directory.test.id}"
}
resource "aws_workspaces_ip_group" "b" {
directory_id = "${aws_workspaces_directory.test.id}"
}
I know AWS Terraform provider tries to follow AWS API as close as possible, but this option looks more explicit than implicit group disassociation.
@DrFaust92 @bflad @gdavison What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @Tensho. I personally prefer the attachment resource option, this would be more intuitive and most similar to other resources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bensunny1 TBH, I can't grasp how attachment may solve the problem I mentioned above in this thread. If I remove IP group attachment and IP group from the configuration all together, then both resources will be marked as orphaned and deleted in undesired order (first IP group, then IP group attachment) by Terraform. I guess it's OK for other attachments and target resources, e.g. aws_iam_role_policy_attachment
and aws_iam_role
/aws_iam_policy
, but it doesn't work for IP group API. Or maybe I don't understand how Terraform builds the graph nodes for destroy
operations...
@apparentlymart @bflad Please help us to sort out this chicken-egg question 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tensho You're right, #14451 (comment) i think is a pretty good idea and deals with the problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the best option here is to add the DisassociateIpGroups()
call to the aws_workspaces_ip_group
delete operation as well. There isn't currently a way to model these mutual relationships in Terraform, so we're stuck with brute-forcing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gdavison Got it. Added ip group disassociation from a directory on delete.
@DrFaust92 Fixed review comments, please take a look now 🙇♂️ |
4411593
to
62283f6
Compare
62283f6
to
d719ba8
Compare
d719ba8
to
6009892
Compare
4c0148e
to
5c8e86d
Compare
5c8e86d
to
09d496a
Compare
163dce4
to
b7f3204
Compare
|
8c77a62
to
b18e872
Compare
b18e872
to
5ba0efd
Compare
5ba0efd
to
69810af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
--- PASS: TestAccAwsWorkspacesIpGroup_disappears (65.56s)
--- PASS: TestAccAwsWorkspacesIpGroup_basic (113.13s)
--- PASS: TestAccAwsWorkspacesIpGroup_tags (128.86s)
--- PASS: TestAccAwsWorkspacesDirectory_subnetIds (727.54s)
--- PASS: TestAccAwsWorkspacesDirectory_disappears (747.15s)
--- PASS: TestAccAwsWorkspacesDirectory_workspaceCreationProperties (747.94s)
--- PASS: TestAccAwsWorkspacesDirectory_selfServicePermissions (749.95s)
--- PASS: TestAccAwsWorkspacesIpGroup_MultipleDirectories (761.30s)
--- PASS: TestAccAwsWorkspacesDirectory_basic (771.07s)
--- PASS: TestAccAwsWorkspacesDirectory_ipGroupIds (794.51s)
--- PASS: TestAccAwsWorkspacesDirectory_tags (818.72s)
This has been released in version 3.17.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks! |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
Community Note
Relates #434
Closes #14669
Release note for CHANGELOG:
Acceptance Test